refactor: remove unnecessary ReactElement type annotations in components#40821
Conversation
|
|
Important Review skippedToo many files! This PR contains 196 files, which is 46 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (196)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Looks like this PR is ready to merge! 🎉 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40821 +/- ##
===========================================
- Coverage 70.16% 70.08% -0.08%
===========================================
Files 3342 3340 -2
Lines 123685 123562 -123
Branches 22077 22083 +6
===========================================
- Hits 86778 86601 -177
- Misses 33561 33613 +52
- Partials 3346 3348 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
be9e153 to
d91224d
Compare
- Updated multiple components to remove explicit ReactElement return type annotations, simplifying the code. - Adjusted function signatures in various files to use implicit return types instead. - Improved consistency across the codebase by standardizing function definitions.
d91224d to
09076e7
Compare
There was a problem hiding this comment.
No issues found across 196 files
Note: This PR contains a large number of files. cubic only reviews up to 100 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
Re-trigger cubic
| }; | ||
|
|
||
| const ConferencePage = (): ReactElement => { | ||
| const ConferencePage = () => { |
There was a problem hiding this comment.
DOM-based Cross-Site Scripting (XSS) and Open Redirect via callUrl Parameter in ConferencePage
The ConferencePage component is vulnerable to DOM-based Cross-Site Scripting (XSS) and Open Redirect because it retrieves the callUrl parameter directly from the URL query string without validation or sanitization, and subsequently passes it to window.open().
Specifically, getQueryParams extracts callUrl from window.location.search. This value is passed into useVideoConfOpenCall, which invokes window.open(callUrl). If an attacker crafts a link containing a javascript: protocol (e.g., javascript:alert(document.cookie)//), the browser will execute the arbitrary JavaScript payload in the context of the Rocket.Chat application origin when the victim visits the page. Alternatively, an attacker can supply an external malicious URL to perform an Open Redirect attack.
This vulnerability also transitively affects the Outlook Calendar integration (OutlookCalendarEventModal.tsx), where a malicious meetingUrl synced from a calendar event is passed to useVideoConfOpenCall and opened without validation.
Steps to Reproduce
- As an attacker, craft a malicious link targeting the Rocket.Chat instance:
https://<your-rocketchat-domain>/conference?callUrl=javascript:alert(document.domain)// - Send this link to an authenticated Rocket.Chat user.
- Once the user clicks the link, the
ConferencePagecomponent loads, retrieves thecallUrlparameter, appends user details, and executeswindow.open('javascript:alert(document.domain)//&name=...'). - The JavaScript payload executes, displaying an alert with the application's domain.
Trace
graph TD
subgraph SG0 ["apps/meteor/client/components/LoadingIndicator.tsx"]
LoadingIndicator["Displays a loading animation with configurable size variation."]
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["apps/meteor/client/views/conference/ConferencePage.tsx"]
getQueryParams["getQueryParams"]
ConferencePage{{"Component that handles the video conference page routing and call initialization."}}
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["apps/meteor/client/views/conference/ConferencePageError.tsx"]
ConferencePageError["Error state component for the video conference page."]
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG3 ["apps/meteor/client/views/conference/ConferenceRoute.tsx"]
ConferenceRoute["Route component that enforces authentication for video conferences."]
end
style SG3 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG4 ["apps/meteor/client/views/room/contextualBar/VideoConference/VideoConfBlockModal.tsx"]
VideoConfBlockModal["Modal component shown when a browser blocks a video conference call popup."]
end
style SG4 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG5 ["apps/meteor/client/views/room/contextualBar/VideoConference/hooks/useVideoConfOpenCall.tsx"]
useVideoConfOpenCall["Hook to open a video conference call, handling browser popup blocking."]
open["Internal function to open a URL in a new window."]
end
style SG5 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG6 ["apps/meteor/client/views/root/PageLoading.tsx"]
PageLoading["Displays a loading spinner or indicator while a page is loading."]
end
style SG6 fill:#2a2a2a,stroke:#444,color:#aaa
ConferencePage --> ConferencePageError
ConferencePage --> useVideoConfOpenCall
ConferencePage --> PageLoading
ConferencePage --> getQueryParams
useVideoConfOpenCall --> open
useVideoConfOpenCall --> VideoConfBlockModal
PageLoading --> LoadingIndicator
open --> open
ConferenceRoute --> ConferencePage
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/client/views/conference/ConferencePage.tsx
Lines: 17
Severity: high
Vulnerability: DOM-based Cross-Site Scripting (XSS) and Open Redirect via callUrl Parameter in ConferencePage
Description:
The `ConferencePage` component is vulnerable to DOM-based Cross-Site Scripting (XSS) and Open Redirect because it retrieves the `callUrl` parameter directly from the URL query string without validation or sanitization, and subsequently passes it to `window.open()`.
Specifically, `getQueryParams` extracts `callUrl` from `window.location.search`. This value is passed into `useVideoConfOpenCall`, which invokes `window.open(callUrl)`. If an attacker crafts a link containing a `javascript:` protocol (e.g., `javascript:alert(document.cookie)//`), the browser will execute the arbitrary JavaScript payload in the context of the Rocket.Chat application origin when the victim visits the page. Alternatively, an attacker can supply an external malicious URL to perform an Open Redirect attack.
This vulnerability also transitively affects the Outlook Calendar integration (`OutlookCalendarEventModal.tsx`), where a malicious `meetingUrl` synced from a calendar event is passed to `useVideoConfOpenCall` and opened without validation.
Proof of Concept:
1. As an attacker, craft a malicious link targeting the Rocket.Chat instance:
`https://<your-rocketchat-domain>/conference?callUrl=javascript:alert(document.domain)//`
2. Send this link to an authenticated Rocket.Chat user.
3. Once the user clicks the link, the `ConferencePage` component loads, retrieves the `callUrl` parameter, appends user details, and executes `window.open('javascript:alert(document.domain)//&name=...')`.
4. The JavaScript payload executes, displaying an alert with the application's domain.
Affected Code:
- [Rocket.Chat/apps/meteor/client/views/conference/ConferencePage.tsx:9-15](https://github.com/RocketChat/Rocket.Chat/blob/master/apps/meteor/client/views/conference/ConferencePage.tsx#L9-L15)
```typescript
const getQueryParams = () => {
const queryString = window.location.search;
const urlParams = new URLSearchParams(queryString);
const callUrlParam = urlParams.get('callUrl');
return { callUrlParam };
};
```
- [Rocket.Chat/apps/meteor/client/views/conference/ConferencePage.tsx:24-25](https://github.com/RocketChat/Rocket.Chat/blob/master/apps/meteor/client/views/conference/ConferencePage.tsx#L24-L25)
```typescript
const { callUrlParam } = getQueryParams();
const callUrl = callUrlParam && userDisplayName ? `${callUrlParam}&name=${userDisplayName}` : callUrlParam;
```
- [Rocket.Chat/apps/meteor/client/views/conference/ConferencePage.tsx:32](https://github.com/RocketChat/Rocket.Chat/blob/master/apps/meteor/client/views/conference/ConferencePage.tsx#L32)
```typescript
handleOpenCall(callUrl);
```
- [Rocket.Chat/apps/meteor/client/views/room/contextualBar/VideoConference/hooks/useVideoConfOpenCall.tsx:14-15](https://github.com/RocketChat/Rocket.Chat/blob/master/apps/meteor/client/views/room/contextualBar/VideoConference/hooks/useVideoConfOpenCall.tsx#L14-L15)
```typescript
const open = () => window.open(callUrl);
const popup = open();
```
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
Proposed changes (including videos or screenshots)
As a first step towards upgrading to React 19, it handles types from
@types/reactlooking forward the next major.Issue(s)
Task: ARCH-2170
Steps to test or reproduce
Further comments
No runtime change is expected from it.